I've tested this on 3.x (cherry-picked to my fork here: https://github.com/akien-mga/godot/tree/3.x-meshlib-transform), it seems to work well 👍

I'm pretty new to using GridMaps but I ran into this limitation right away so I'm glad we're improving this.

Workflow wise, I'm not sure I'm fully happy with what we have even after this PR though, as there's still a lot of manual work needed to edit the MeshLibrary after conversion, unless I missed something obvious. For example, the MeshInstance transforms from my source scene are not included in the MeshLibrary resource; I need to redefine them manually in the resource.

Also (not related to this PR but it's another similar workflow issue), some material properties seem not to be properly included in the MeshLibrary resource (in my case, disabled Cull Mode).

This might warrant a dedicated proposal though, I'm just documented this here so that other GridMap users can confirm that as of this PR, manual edition of the MeshLibrary would still be required.

Also (not related to this PR but it's another similar workflow issue), some material properties seem not to be properly included in the MeshLibrary resource (in my case, disabled Cull Mode).

Actually this is a different issue, it seems the Cull Mode does get included properly when converting to MeshLibrary. It's just an issue with edited MeshLibrary resources not properly getting reloaded in the editor, one needs to restart the editor to see the changes (another bug to file).

So it does seem that it could be expected for the "Convert to MeshLibrary" action to also bake the MeshInstance's transform into the MeshLibrary, so that it doesn't need to be redone manually. WDYT?

It's worth noting that users might be changing the position of their MeshInstances in the source scene for cosmetic purposes, to avoid having everything crammed on origin. If we bake the position in the MeshLibrary, this might be problematic for some. But at the same time, one can use Spatial/Node3D nodes for this cosmetic positioning, and keep the MeshInstance transform strictly for things which should be included in the MeshLibrary. Both workflows can be seen as valid so if implemented, this could be made into a checkbox in the "Convert to MeshLibrary" dialog.

It's worth noting that users might be changing the position of their MeshInstances in the source scene for cosmetic purposes, to avoid having everything crammed on origin. If we bake the position in the MeshLibrary, this might be problematic for some. But at the same time, one can use Spatial/Node3D nodes for this cosmetic positioning, and keep the MeshInstance transform strictly for things which should be included in the MeshLibrary. Both workflows can be seen as valid so if implemented, this could be made into a checkbox in the "Convert to MeshLibrary" dialog.

I 100% agree, but wasn't entirely sure how best to implement it. Looking at the code of the importer now, it is already setup to allow each MeshInstance to have a different parent - so I'd agree that using that as a cosmetic intermediate could work well. I'll get on that ASAP.

I 100% agree, but wasn't entirely sure how best to implement it. Looking at the code of the importer now, it is already setup to allow each MeshInstance to have a different parent - so I'd agree that using that as a cosmetic intermediate could work well. I'll get on that ASAP.

That sounds great! For compatibility's sake, I would suggest adding a checkbox in the convert dialog (next to "Merge With Existing") to allow toggling off this new behavior. I can imagine that some users might have pretty big mesh libraries where they don't want the transform baked into the MeshLibrary. I'd make it opt-out (enabled by default) though as it's the better workflow for new users IMO.

Edit: At least for 3.4 - for 4.0 we could go with only the new workflow. I guess we can decide based on user feedback on the new workflow.

That sounds great! For compatibility's sake, I would suggest adding a checkbox in the convert dialog (next to "Merge With Existing") to allow toggling off this new behavior.

Yep, that's how I'm handling it. However, this menu here is currently causing me some headache

I'm making it so that, if you chose "Import from Scene (Apply Transforms)" for that MeshLibrary, then "Update from Scene" will always automatically include the transforms, but I'm not entirely sure that's the right approach.

I'm making it so that, if you chose "Import from Scene (Apply Transforms)" for that MeshLibrary, then "Update from Scene" will always automatically include the transforms, but I'm not entirely sure that's the right approach.

Ah I never used that menu, I usually use the "Convert to MeshLibrary" option instead of doing it the other way around.

Maybe the "Update from Scene" dialog could have two OK buttons, one to apply with and one without transforms:

Or a checkbox in the dialog. Leaving it as a decision on update might be simpler than saving in some project metadata that the original import was done with or without transforms -- though this seems to already rely on saving some information on what the source scene was, so that could be saved alongside it.

Or a checkbox in the dialog. Leaving it as a decision on update might be simpler than saving in some project metadata that the original import was done with or without transforms -- though this seems to already rely on saving some information on what the source scene was, so that could be saved alongside it.

I went with the metadata approach in the latest commit. It works, but I think I'll add a checkbox to the Update dialog anyway to make things clearer.

EDIT: It should more or less be working now. Marking as draft until I've figured out some UI issues.

EDIT 2: Okay, this is how I handled that menu:

And the Update prompt:

Should all work as intended now.

Thanks!

Cherry-picked for 3.4.